Skip to content

Conversation

Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Apr 29, 2025

Move building the .mod files from openmp/flang to openmp/flang-rt using a shared mechanism. Motivations to do so are:

  1. Most modules are target-dependent and need to be re-compiled for each target separate, which is something the LLVM_ENABLE_RUNTIMES system already does. Prime example is iso_c_binding.mod which encodes the target's ABI. Most other modules have #ifdef-enclosed code as well.

  2. CMake has support for Fortran that we should use. Among other things, it automatically determines module dependencies so there is no need to hardcode them manually.

  3. It allows using Fortran itself to implement Flang-RT. Currently, only iso_fortran_env_impl.f90 emits object files that are needed by Fortran applications ([flang] Linker for non-constant accesses to kind arrays (integer_kind, logical_kind, real_kind) #89403). The workaround of [flang][runtime] Build ISO_FORTRAN_ENV to export kind arrays as linkable symbols #95388 could be reverted.

Some new dependencies come into play:

  • openmp depends on flang-rt for building lib_omp.mod and lib_omp_kinds.mod. Currently, if flang-rt is not found then the modules are not built
  • check-flang depends on flang-rt: If not found, the majority of tests are disabled. If not building in a bootstrpping build, the location of the module files can be pointed to using -DFLANG_INTRINSIC_MODULES_DIR=<path>, e.g. in a flang-standalone build. Alternatively, the test needing any of the instrinsic modules could be marked with REQUIRES: flangrt-modules which would affect ~217 files.
  • check-flang depends on openmp: Not a change; tests requiring lib_omp.mod and lib_omp_kinds.mod those are already marked with openmp_runtime.

As intrinsic are now specific to the target, their location os moved from include/flang to <resource-dir>/finclude/<triple>. The mechnism to compute the location have been moved from flang-rt (previously used to compute the location of libflang_rt.*.a) to common locations in cmake/GetToolchainDirs.cmake and runtimes/CMakeLists.txt so they can be used by both, openmp and flang-rt. Potentially the mechnism could also be shared by other libraries such as compiler-rt.

finclude was chosen because gfortran uses it as well and avoids misuse such as #include <flang/iso_c_binding.mod>. The search location is now determined by ToolChain in the driver, instead of by the frontend. Now the driver adds -fintrinsic-module-path for that location to the frontend call (Just like gcc does). -fintrinsic-module-path had to be fixed for this because ironically it was only added to searchDirectories, but not intrinsicModuleDirectories_. Since the driver determines the location, tests invoking flang -fc1 and bbc must also be passed the location by llvm-lit. This works like llvm-lit does for finding the include dirs for Clang using -print-file-name=....

Related PRs:

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the main limitation here? If this is just a file dependency it should be identical to how all the OpenMP tests depend on omp.h being in the resource directory. IMHO this is trivial if we do a runtimes build, since we can just require that openmp;flang-rt are in the same toolchain, which then gives us well defined access to openmp's CMake targets so long as it's listed before flang-rt.

@Meinersbur
Copy link
Member Author

While I appreciate the review, it is not yet in the state that warants one. It is still in an experimentation stage, so I did not yet care about formatting. There are also a lot of changes in here that will eventually not be needed.

Goals are:

  1. Currently modules files are expected at $prefix/include/flang/*.mod where prefix is the parent of bin where flang is located. It should be in $prefix/lib/clang/finclude/<triple>/*.h, i.e. the resource directory since mod-files are specific to the version of flang, and distinct for each target triple since the mod files can be different for each target. Necessary for cross-compilation. In addition to the CMake code, the flang driver code needs to change as well because it hardcodes $path/../include/flang.

  2. Use cmake to build the module files within the flang-rt/ runtime. The LLVM_ENABLE_RUNTIMES system handles which target triples to build and Flang being available. CMake should care about the build dependencies. Have to change the driver again to tell it where to emit the module files.

  3. Use the same mechanism as above to build omp_lib.mod and omp_lib_kinds.mod, but in the openmp/ runtime. Since in the same CMake builddir, CMake will ensure dependencies already.

  4. This means flang-rt (and openmp) is necessary to be compiled before running flang's tests which require those module files. Flang's OpenMP tests already require openmp's modules to be compiled, it will be no different to flang-rt's builtin modules.

Sounds relatively simple, but there have been many small issues, starting with CMake's misspelling of CMAKE_Fortran_BUILDING_INSTRINSIC_MODULES.

@DanielCChen
Copy link
Contributor

DanielCChen commented May 7, 2025

  1. It should be in $prefix/lib/clang/finclude/<triple>/*.h

Just want to make sure: Should it be $prefix/lib/clang/${LLVM_VERSION_MAJOR}/finclude/<triple>/*.mod?

@Meinersbur
Copy link
Member Author

Just want to make sure: Should it be $prefix/lib/clang/${LLVM_VERSION_MAJOR}/finclude/<triple>/*.mod?

That is correct, I forgot the version number that is part of the resource directory.

@Meinersbur Meinersbur force-pushed the flang_builtin-mods branch from 907d3d5 to 839198d Compare July 17, 2025 13:04
@github-actions
Copy link

github-actions bot commented Jul 17, 2025

✅ With the latest revision this PR passed the Python code formatter.

@Meinersbur
Copy link
Member Author

What's the main limitation here? If this is just a file dependency it should be identical to how all the OpenMP tests depend on omp.h being in the resource directory.

omp.h is created by configure_file at configure time. No dependency other than runtimes-configure needed.

IMHO this is trivial if we do a runtimes build, since we can just require that openmp;flang-rt are in the same toolchain,

With toolchain you mean a bootstrapping build with LLVM_ENABLE_RUNTIMES=openmp;flang-rt ? Don't forget the users of a Flang-standalone build (cmake -S <llvm-project>/flang).

which then gives us well defined access to openmp's CMake targets so long as it's listed before flang-rt.

check-flang (LLVM_ENABLE_PROJECTS=flang) needs access to libomp.mod (LLVM_ENABLE_RUNTIMES=openmp) and the flang modules as well to work.

@Meinersbur Meinersbur requested a review from kkwli July 17, 2025 13:25
@Meinersbur Meinersbur marked this pull request as ready for review July 19, 2025 12:32
@Meinersbur Meinersbur requested a review from a team as a code owner July 19, 2025 12:32
@sscalpone sscalpone requested review from klausler and vzakhari July 20, 2025 09:25
@Meinersbur Meinersbur requested a review from petrhosek July 25, 2025 13:31
Meinersbur added a commit that referenced this pull request Jul 25, 2025
The default build of openmp (`cmake -S <llvm-project>/runtimes
-DLLVM_ENABLE_RUNTIMES=openmp`) current fails with
```
CMake Error at /home/meinersbur/src/llvm/flangrt/_src/cmake/Modules/GetClangResourceDir.cmake:17 (string):
  string sub-command REGEX, mode MATCH needs at least 5 arguments total to
  command.
Call Stack (most recent call first):
  /home/meinersbur/src/llvm/flangrt/_src/openmp/CMakeLists.txt:126 (get_clang_resource_dir)
```
The reason is that because it is not a bootstrapping-build, the clang
resource dir that it intends to write files such as `omp-tools.h` into,
is unavailable. Using the Clang resource dir for writing files is
conceptually broken, as that dir might be located in
`/usr/lib/clang/<version>/`. Writing to it is only intended in
bootstrapping builds where Clang is built alongside openmp.

This patch unifies the identification of being in a bootstrapping built.
The same `LLVM_TREE_AVAILABLE` definition is going to be used in
#137828. No reason for each runtime to define its own.
@Meinersbur Meinersbur requested a review from ldionne July 26, 2025 13:10
mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Jul 28, 2025
The default build of openmp (`cmake -S <llvm-project>/runtimes
-DLLVM_ENABLE_RUNTIMES=openmp`) current fails with
```
CMake Error at /home/meinersbur/src/llvm/flangrt/_src/cmake/Modules/GetClangResourceDir.cmake:17 (string):
  string sub-command REGEX, mode MATCH needs at least 5 arguments total to
  command.
Call Stack (most recent call first):
  /home/meinersbur/src/llvm/flangrt/_src/openmp/CMakeLists.txt:126 (get_clang_resource_dir)
```
The reason is that because it is not a bootstrapping-build, the clang
resource dir that it intends to write files such as `omp-tools.h` into,
is unavailable. Using the Clang resource dir for writing files is
conceptually broken, as that dir might be located in
`/usr/lib/clang/<version>/`. Writing to it is only intended in
bootstrapping builds where Clang is built alongside openmp.

This patch unifies the identification of being in a bootstrapping built.
The same `LLVM_TREE_AVAILABLE` definition is going to be used in
llvm#137828. No reason for each runtime to define its own.
@Meinersbur
Copy link
Member Author

ping

@Meinersbur
Copy link
Member Author

Should we choose a different module path than finclude to avoid accidental confusion between flang and gfortran modules files, e.g. fmod?

Comment on lines +342 to +361
elemental logical function ieee_is_finite_a2(x); real(2), intent(in) :: x;
!dir$ ignore_tkr(d) x;
end function ieee_is_finite_a2;
elemental logical function ieee_is_finite_a3(x); real(3), intent(in) :: x;
!dir$ ignore_tkr(d) x;
end function ieee_is_finite_a3;
elemental logical function ieee_is_finite_a4(x); real(4), intent(in) :: x;
!dir$ ignore_tkr(d) x;
end function ieee_is_finite_a4;
elemental logical function ieee_is_finite_a8(x); real(8), intent(in) :: x;
!dir$ ignore_tkr(d) x;
end function ieee_is_finite_a8;
elemental logical function ieee_is_finite_a10(x); real(10), intent(in) :: x;
!dir$ ignore_tkr(d) x;
end function ieee_is_finite_a10;
#if FLANG_SUPPORT_R16
elemental logical function ieee_is_finite_a16(x); real(16), intent(in) :: x;
!dir$ ignore_tkr(d) x;
end function ieee_is_finite_a16;
#endif
Copy link
Member

@DavidTruby DavidTruby Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change related to the move of the .mod files? It looks like a separate issue to me at a glance

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workaround for #139297. Will not be necessary after landing #143699. See summary.


# CMake 3.24 is the first version of CMake that directly recognizes Flang.
# LLVM's requirement is only CMake 3.20, teach CMake 3.20-3.23 how to use Flang, if used.
if (CMAKE_VERSION VERSION_LESS "3.24")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flang support in CMake wasn't really complete/fully functional until 3.28 so I think this should be bumped up to that. Unless you've specifically tested with 3.24 and it doesn't hit the edge cases we didn't fix until later?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I tests 3.24 already. I boiled down to CMAKE_Fortran_PREPROCESS_SOURCE not being defined. I will test again before landing.

Copy link
Member

@DavidTruby DavidTruby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM other than my concerns about the specific CMake version where flang was properly supported. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants